Repository navigation
Report the outcome of reminders and shares to notes instead of guessing it - #6717
AndyScherzinger wants to merge 7 commits into
Conversation
1cff49c to
93bea38
Compare
5099081 to
f8ce16e
Compare
93bea38 to
0e97e99
Compare
f8ce16e to
36e1a8e
Compare
0e97e99 to
0f19149
Compare
36e1a8e to
ba3d4b7
Compare
0f19149 to
bf571a9
Compare
ba3d4b7 to
ca9e10d
Compare
bf571a9 to
a0740e5
Compare
75b1f11 to
fdb65eb
Compare
a0740e5 to
08df497
Compare
fdb65eb to
4db6b67
Compare
08df497 to
00d3a61
Compare
4db6b67 to
77b4de6
Compare
00d3a61 to
8200d2c
Compare
77b4de6 to
24c3354
Compare
8200d2c to
57e7e04
Compare
24c3354 to
007ce89
Compare
57e7e04 to
2fb50b7
Compare
007ce89 to
18a84d3
Compare
2fb50b7 to
85883e2
Compare
18a84d3 to
43377cb
Compare
85883e2 to
c7fd3fb
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🔵 Low · up to A Notes share may not complete even though the chat shows “Message sent,” and an uncertain outcome is not reported. Correct the lookup and feedback paths before merging unless that bounded risk is accepted. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 12.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 6 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly summarizes the main change: reporting actual outcomes for reminders and shares to notes. Description check The description explains the motivation, implementation, retry behavior, testing, and checklist status. It omits the template's TODO section, but the description is otherwise complete. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 12.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 6 files. (1 skipped: 1 unsupported.)
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fbe4d5e8-5473-4715-bd02-27cbbd313487
📒 Files selected for processing (7)
app/src/main/java/com/nextcloud/talk/api/NcApiCoroutines.ktapp/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/ChatNetworkDataSource.ktapp/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.ktapp/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktapp/src/main/res/values/strings.xmlapp/src/test/java/com/nextcloud/talk/chat/viewmodels/ChatViewModelTest.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
| shareToNotes("share the location to notes", credentials, url) { referenceId -> | ||
| chatNetworkDataSource.shareLocationToNotes(credentials, url, objectType, objectId, metadata, referenceId) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
rg -n 'shareLocationToNotes|shareToNotes\(|getUrlToSendLocation|getUrlForChat|alreadyShared|pullChatMessages' app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt app/src/main/java/com/nextcloud/talk/api/ApiUtils.kt
sed -n '2150,2252p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktRepository: nextcloud/talk-android
Length of output: 9236
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- ApiUtils declarations ---'
rg -n -F --glob '*.kt' -- 'fun getUrlToSendLocation' .
rg -n -F --glob '*.kt' -- 'fun getUrlForChat(' .
printf '%s\n' '--- URL helper implementations ---'
for f in $(rg -l -F --glob '*.kt' -- 'fun getUrlToSendLocation' .); do
file="$f"
start=$(rg -n -F -- 'fun getUrlToSendLocation' "$file" | cut -d: -f1 | head -1)
end=$((start + 22))
sed -n "${start},${end}p" "$file"
done
for f in $(rg -l -F --glob '*.kt' -- 'fun getUrlForChat(' .); do
file="$f"
start=$(rg -n -F -- 'fun getUrlForChat(' "$file" | cut -d: -f1 | head -1)
end=$((start + 18))
sed -n "${start},${end}p" "$file"
done
printf '%s\n' '--- ChatActivity location-share caller ---'
sed -n '3870,3955p' app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt
printf '%s\n' '--- Retrofit data-source methods and API declarations ---'
sed -n '80,155p' app/src/main/java/com/nextcloud/talk/chat/data/network/RetrofitChatNetwork.kt
rg -n -F --glob '*.kt' -- 'shareLocationToNotes' app/src/main/java
rg -n -F --glob '*.kt' -- 'pullChatMessages' app/src/main/java/com/nextcloud/talk/chat/data/networkRepository: nextcloud/talk-android
Length of output: 10182
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- coroutine API bindings ---'
rg -n -F --glob '*.kt' -- 'fun sendLocation' app/src/main/java
rg -n -F --glob '*.kt' -- 'fun pullChatMessages' app/src/main/java
printf '%s\n' '--- API declaration context ---'
for f in $(rg -l -F --glob '*.kt' -- 'fun sendLocation' app/src/main/java); do
line=$(rg -n -F -- 'fun sendLocation' "$f" | head -1 | cut -d: -f1)
sed -n "$((line-8)),$((line+14))p" "$f"
done
for f in $(rg -l -F --glob '*.kt' -- 'fun pullChatMessages' app/src/main/java); do
line=$(rg -n -F -- 'fun pullChatMessages' "$f" | head -1 | cut -d: -f1)
sed -n "$((line-8)),$((line+18))p" "$f"
done
printf '%s\n' '--- holdsShare and transient retry flow ---'
rg -n -F -- 'fun holdsShare' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
sed -n '2190,2265p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktRepository: nextcloud/talk-android
Length of output: 7481
🏁 Script executed:
sed -n '2718,2745p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.ktRepository: nextcloud/talk-android
Length of output: 1533
Use the Notes chat URL for the delivery lookup.
getUrlToSendLocation appends /share to the chat URL. ChatActivity passes that URL to shareLocationToNotes, and shareToNotes reuses it for alreadyShared. The lookup therefore sends pullChatMessages to the location-share endpoint instead of the note-to-self chat endpoint.
The lookup can fail or return no message data. alreadyShared and holdsShare treat both cases as possibly delivered, so the retry is skipped. The user may then see only the immediate “Message sent” snackbar.
Pass the Notes chat URL as a separate lookup URL. Keep the /share URL for the location-share request.
| if (!alreadyShared(credentials, notesUrl, referenceId)) { | ||
| attemptShare(share, referenceId)?.let { reportShareFailure(description, it) } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
An inconclusive lookup ends the flow without telling the user.
If alreadyShared returns true because the lookup failed or returned 304, the coroutine ends with no emission. The share may not have arrived, but the chat has already shown "Message sent". The PR goal is to report the outcome instead of guessing it. Skipping the retry is correct here, but the result is still uncertain. Report that uncertainty, for example with the existing failure string or a "may not have been added" string, when the lookup could not confirm the share.
| @Suppress("Detekt.TooGenericExceptionCaught") | ||
| private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) { | ||
| viewModelScope.launch { | ||
| try { | ||
| request() | ||
| } catch (e: Exception) { | ||
| logger.e(TAG, "Failed to $description", e) | ||
| _actionFailures.tryEmit(failureMessage) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1855,1905p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
sed -n '2240,2275p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
sed -n '1520,1550p' app/src/main/java/com/nextcloud/talk/chat/ChatActivity.ktRepository: nextcloud/talk-android
Length of output: 5273
🏁 Script executed:
set -o pipefail
printf '%s\n' '--- imports and relevant declarations ---'
sed -n '1,90p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
sed -n '1860,1900p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
sed -n '2248,2275p' app/src/main/java/com/nextcloud/talk/chat/viewmodels/ChatViewModel.kt
printf '%s\n' '--- reminder call sites ---'
rg -n -F --glob '*.kt' -- 'getReminder(' app/src/main/java
rg -n -F --glob '*.kt' -- 'setReminder(' app/src/main/java
rg -n -F --glob '*.kt' -- 'deleteReminder(' app/src/main/java
printf '%s\n' '--- actionFailures collector owner and lifecycle context ---'
rg -n -F --glob 'ChatActivity.kt' -- 'actionFailures' app/src/main/java
sed -n '1470,1575p' app/src/main/java/com/nextcloud/talk/chat/ChatActivity.kt
printf '%s\n' '--- view-model scope and activity lifecycle declarations ---'
rg -n -F --glob '*.kt' -- 'class ChatActivity' app/src/main/java
rg -n -F --glob '*.kt' -- 'onCleared(' app/src/main/java/com/nextcloud/talk/chatRepository: nextcloud/talk-android
Length of output: 16110
Preserve cancellation in both reminder handlers.
Both handlers catch CancellationException through Exception. requestWithFeedback can report cancellation through _actionFailures, while getReminder resets the state to GetReminderStartState. The snackbar is conditional because ChatActivity collects the flow in lifecycleScope; it may not appear if the activity is being destroyed. The cancellation is still incorrectly swallowed.
Suggested fix
+ } catch (e: CancellationException) {
+ throw e
} catch (e: Exception) {Add this cancellation-first catch before the existing Exception catch in both requestWithFeedback and getReminder.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Suppress("Detekt.TooGenericExceptionCaught") | |
| private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) { | |
| viewModelScope.launch { | |
| try { | |
| request() | |
| } catch (e: Exception) { | |
| logger.e(TAG, "Failed to $description", e) | |
| _actionFailures.tryEmit(failureMessage) | |
| } | |
| } | |
| @Suppress("Detekt.TooGenericExceptionCaught") | |
| private fun requestWithFeedback(description: String, @StringRes failureMessage: Int, request: suspend () -> Unit) { | |
| viewModelScope.launch { | |
| try { | |
| request() | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Exception) { | |
| logger.e(TAG, "Failed to $description", e) | |
| _actionFailures.tryEmit(failureMessage) | |
| } | |
| } |
Four call sites had each grown their own copy of "apply locally, retry once on a transient failure, revert when it finally fails, revert when cancelled", with the retry predicate spelled out twice and one of them shipping a guard bug. optimisticAction() now carries that shape, including the case where the server refuses in the payload rather than in the status code, and the case where an answer means the change was already applied - which must not revert anything. No behaviour change; the repository tests of reactions, deletions, edits, pinning and dismissals carry the migration. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Marking a conversation read or unread, favoriting, archiving and the important and sensitive toggles now go through the shared optimistic helper, so every one of them retries a connection problem once and takes its change back when the screen is left mid-request. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The notification level, the message expiration and the call notification switch logged their failure and left the screen showing a setting the server never accepted. They now go back to the previous value and say so. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
… once Deleting left the row in place until the worker had finished and the list had been fetched again. It now disappears on confirmation, reusing the guard that hides a conversation being left, and comes back if the delete fails. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
The helper was only exercised through the repositories that use it, so the rules every optimistic action depends on - applied before the answer, taken back on a refusal or a cancellation, kept after one hiccup - had no test of their own. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
Both used an Observable with a disposable that the chat throws away when it is paused, so backgrounding the app while the request was in flight dropped it silently. They now run in the view model scope like every other request that has to finish. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
A reminder the server refused was only logged, and the chat says "Message sent" the moment a share to notes starts, before the answer arrives - so a share that failed was reported as a success. Both now show what happened. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Andy Scherzinger <info@andy-scherzinger.de>
43377cb to
a238355
Compare
c7fd3fb to
dcc3a79
Compare
a238355 to
09db8f2
Compare
09db8f2 to
c43e42d
Compare
1 participant
Stacked on #6715 — review that one first. #6694 merged already; this branch targets
feat/noid/optimisticConversationActionsand will be retargeted to master once #6715 lands.Two things in the chat reported the wrong outcome, and both sat behind the last RxJava subscriptions in
ChatViewModelthat still used thedisposableSetthe chat throws away on pause.refactor(chat): move reminders and sharing to notes off RxJava—setReminder,getReminder,deleteReminder,shareToNotesandshareLocationToNotesmove down throughChatNetworkDataSourceandRetrofitChatNetworkonto four newNcApiCoroutinesendpoints. This is not the pure refactor it looks like:disposableSetis disposed inonPause, not inonCleared, so backgrounding the chat while one of these requests was in flight cancelled it silently. OnviewModelScopethey finish.getCapabilities,joinRoomandcreateRoomstay on RxJava on purpose. They sit in that samedisposableSet, so moving them is the same behaviour change — and for a join or a capability fetch, what should happen when the chat is backgrounded mid-request is a decision, not a refactor. Neither blocks anything.fix(chat): tell the user when a reminder or a share to notes failed— a reminder the server refused was written to the log and nowhere else. AndshowSnackBarfires "Message sent" the moment a share to notes is started, before the answer arrives, so a share that failed was reported to the user as a success. Both now report the failure, through theSharedFlowand snackbar shape #6694 established for reaction failures.A share that failed for a transient reason is retried, but never blindly. Talk stores a message's
referenceIdand never looks at it again — verified against a Nextcloud 36 dev instance with Talkmain: posting twice with the same reference id returns201twice and leaves two messages in the room. So before a second attempt the client asks the note to self whether the message arrived anyway, and only sends it again when it did not. A lookup that cannot be answered counts as "it may be there", because a duplicate note is worse than a missing retry. The location share now carries a reference id too, which theshareendpoint has always accepted.Deliberately without an optimistic reminder state, although the implementation plan asked for one:
getReminderExistStatehas exactly one consumer, the reminder dialog header, andDateTimeComposerefetches the reminder in its constructor every time that dialog opens, while Set and Delete dismiss it immediately. An optimistic value would never be rendered.🖼️ Screenshots
No UI was added or changed. One new string,
nc_message_not_added_to_notes, shown in the existing chat snackbar.🏁 Checklist
holdsShare, the rule that decides whether a failed share is sent again.ChatViewModeltakes 14 constructor dependencies plussharedApplication, so there is no view-model-level test for the snackbar itself; the helper contract these paths rely on is covered in Put every optimistic action on one helper and fix the ones that failed silently #6715🤖 AI (if applicable)